Convert file-level rustfmt skip to fn level skips #3809

joostjager · 2025-05-29T13:13:30Z

Extend the approach proposed in #3767 to the full repository. The change set is automatically generated using a script. The script omits skip directives when they would not impact formatting (code already rustfmt-compliant).

I've fixed various problems with the script, and it may be that there are a few edge cases left that should have had a skip directive. There could also be a few redundant ones. I think we can live with that.

Omitted are files that are formatted in in-flight rustfmt PRs: #3783, #3725 ~~, #3338 and #3337~~

ldk-reviews-bot · 2025-05-29T13:13:33Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

joostjager · 2025-05-29T17:35:40Z

As discussed in sync meet, I will try to extend the script to ignore rustfmt changes that only affect the function signature.

joostjager · 2025-05-30T08:43:55Z

Pushed fix up commit to ignore sig changes. Modest reduction in skips, with a few edge cases.

codecov · 2025-06-02T13:22:16Z

Codecov Report

Attention: Patch coverage is 99.01153% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.91%. Comparing base (e797180) to head (a38db49).

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	91.17%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3809      +/-   ##
==========================================
+ Coverage   89.87%   89.91%   +0.03%     
==========================================
  Files         162      162              
  Lines      130214   130684     +470     
  Branches   130214   130684     +470     
==========================================
+ Hits       117034   117508     +474     
+ Misses      10486    10484       -2     
+ Partials     2694     2692       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

Why is this in draft? This needs a rebase by now, too.

tnull · 2025-06-03T11:40:57Z

fuzz/src/msg_targets/mod.rs

@@ -1,48 +1,47 @@
-#![cfg_attr(rustfmt, rustfmt_skip)]


This was put here deliberately as we opted against running rustfmt on autogenerated files. We need to fix it either way, likely just revert it to include the skip.

Ah, didn't know it was auto-generated. Removed the commit that formats this file.

joostjager · 2025-06-03T16:10:31Z

It's in draft because I want to see #3767 merged first. Don't want to spend time on rebase if we don't go this way.

joostjager · 2025-06-11T10:41:27Z

#3767 seems a positive change. New code being added in PRs (for example async payments) is now automatically formatted and not creating more formatting debt. No bad rebase conflicts that I've heard of either.

Enough to go ahead and re-run the script on the latest main.

tnull

LGTM from what I've seen so far.

Mind including the same changes for the remaining files in chain,ln, and routing, too?

joostjager · 2025-06-11T11:15:11Z

LGTM from what I've seen so far.

Mind including the same changes for the remaining files in chain,ln, and routing, too?

Intentionally left these out because there are fmt prs in flight for those files. Links in pr desc

tnull · 2025-06-11T11:19:40Z

Intentionally left these out because there are fmt prs in flight for those files. Links in pr desc

Ah, right. The remaining ones in routing never went far, so now closed them for now. Please include routing here.

joostjager · 2025-06-11T11:45:42Z

Re-added routing files.

TheBlueMatt · 2025-06-11T23:11:00Z

I admit I'm kinda meh oh doing this for everything. There's a handful of files left that are pretty small and can be easily formatted (at least once trampoline PRs land for chain and the router module PRs get picked back up), and for tests I understood the plan to be "move forward, get there soon" (I know, I know, I plan on finishing #3783 this week), so applying skips to everything in tests (which generally aren't regularly touched) just to format them soon seems kinda weird?

For large files that we want to format iteratively (channelmanager, channel, maybe some of chain though many of those files aren't that large, just very actively being edited), this approach makes a ton of sense, for every file I'm not sure that we want to always format iteratively?

joostjager · 2025-06-12T07:58:47Z

I admit I'm kinda meh oh doing this for everything. There's a handful of files left that are pretty small and can be easily formatted (at least once trampoline PRs land for chain and the router module PRs get picked back up), and for tests I understood the plan to be "move forward, get there soon" (I know, I know, I plan on finishing #3783 this week), so applying skips to everything in tests (which generally aren't regularly touched) just to format them soon seems kinda weird?

I see what you mean. Applying skips now and removing them "soon" feels unnecessary. The way I see it though is that "soon" may be quite a bit further out than we'd like. Trampoline for example may take a while. Adding the skips doesn't hurt, they can easily be removed again if someone feels inclined to format a full file, and they add flexibility for devs to selectively format for whatever reason.

For large files that we want to format iteratively (channelmanager, channel, maybe some of chain though many of those files aren't that large, just very actively being edited), this approach makes a ton of sense, for every file I'm not sure that we want to always format iteratively?

I needed to rebase this PR anyway, and while I did I left out the test files. If you feel it is still too much, maybe you can list the exact files that are definitely good for the incremental approach?

joostjager · 2025-06-12T12:44:34Z

Also not sure at all that doing the tests all in one with mass edits is ideal: #3783 (review)

TheBlueMatt

I think the file list here makes sense. I thought there were substantially more changes to onion_payment.rs for trampoline to come but @valentinewallace tells me they're more in outbound_payment.rs which has more active development and thus higher cost to delaying pushing the format skips down in it.

TheBlueMatt · 2025-06-12T18:08:58Z

lightning/src/ln/chan_utils.rs

 			monitor.provide_secret(281474976710655, secrets.last().unwrap().clone()).unwrap();
 			test_secrets!();

 			secrets.push([0; 32]);
-			secrets.last_mut().unwrap()[0..32].clone_from_slice(&<Vec<u8>>::from_hex("c7518c8ae4660ed02894df8976fa1a3659c1a8b4b5bec0c4b872abeba4cb8964").unwrap());
+			secrets.last_mut().unwrap()[0..32].clone_from_slice(
+				&<Vec<u8>>::from_hex(


Do you want to go ahead and break the hex constants out to give them names?

Done. All gave them the same hex name though...

Formatting improvement.

lightning/src/chain/onchaintx.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

lightning/src/ln/outbound_payment.rs

joostjager · 2025-06-12T19:31:48Z

Removed redundant skips. Didn't do the optional improvements (format short fns), because it is probably easiest to keep this big PR "pure" and do the rest in follow ups.

joostjager · 2025-06-12T19:36:13Z

Didn't squash the redundant skip removal commit, in case I need to re-run the script and re-apply the edge case fixes.

TheBlueMatt

11 files changed, 1984 insertions(+), 668 deletions(-)

Holy diff-size batman!

joostjager force-pushed the rustfmt-skip-all branch from 9874e55 to 69a9581 Compare May 29, 2025 13:40

joostjager mentioned this pull request May 29, 2025

Run rustfmt on channelmanager #3767

Merged

joostjager force-pushed the rustfmt-skip-all branch 2 times, most recently from a88062e to 16025f2 Compare May 29, 2025 14:26

joostjager force-pushed the rustfmt-skip-all branch from 16025f2 to 9ff8996 Compare May 30, 2025 08:42

joostjager force-pushed the rustfmt-skip-all branch from 9ff8996 to cf4e480 Compare June 2, 2025 13:00

tnull reviewed Jun 3, 2025

View reviewed changes

joostjager added the rustfmt label Jun 11, 2025

joostjager force-pushed the rustfmt-skip-all branch from cf4e480 to 06eba3b Compare June 11, 2025 10:39

joostjager force-pushed the rustfmt-skip-all branch from 06eba3b to a38db49 Compare June 11, 2025 10:48

tnull reviewed Jun 11, 2025

View reviewed changes

joostjager force-pushed the rustfmt-skip-all branch from a38db49 to f097207 Compare June 11, 2025 11:45

joostjager force-pushed the rustfmt-skip-all branch from f097207 to 1abfab8 Compare June 11, 2025 12:30

joostjager marked this pull request as ready for review June 11, 2025 12:53

ldk-reviews-bot requested a review from valentinewallace June 11, 2025 12:54

joostjager requested a review from tnull June 11, 2025 15:04

joostjager force-pushed the rustfmt-skip-all branch from 1abfab8 to 8cecc34 Compare June 12, 2025 07:57

joostjager force-pushed the rustfmt-skip-all branch from 8cecc34 to 2dda75d Compare June 12, 2025 08:00

joostjager mentioned this pull request Jun 12, 2025

Rustfmt chainmonitor.rs #3847

Open

TheBlueMatt reviewed Jun 12, 2025

View reviewed changes

joostjager force-pushed the rustfmt-skip-all branch 3 times, most recently from 7d69403 to 516fdb3 Compare June 12, 2025 18:42

joostjager added 2 commits June 12, 2025 20:45

Convert file-level rustfmt skip to fn level skips

30ceb31

Extract hex constants in test_per_commitment_storage

6932d3e

Formatting improvement.

joostjager force-pushed the rustfmt-skip-all branch from 516fdb3 to 6932d3e Compare June 12, 2025 18:46

valentinewallace reviewed Jun 12, 2025

View reviewed changes

Remove redundant skips

ee3d225

joostjager requested a review from valentinewallace June 12, 2025 19:32

valentinewallace approved these changes Jun 12, 2025

View reviewed changes

joostjager requested a review from TheBlueMatt June 12, 2025 19:36

valentinewallace mentioned this pull request Jun 12, 2025

Remove low-hanging fruit rustfmt::skips in Channel #3854

Open

TheBlueMatt approved these changes Jun 13, 2025

View reviewed changes

joostjager merged commit 3e9e6a3 into lightningdevkit:main Jun 13, 2025
26 of 27 checks passed

Convert file-level rustfmt skip to fn level skips #3809

Convert file-level rustfmt skip to fn level skips #3809

Uh oh!

Conversation

joostjager commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager commented May 29, 2025

Uh oh!

joostjager commented May 30, 2025

Uh oh!

codecov bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager commented Jun 3, 2025

Uh oh!

joostjager commented Jun 11, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager commented Jun 11, 2025

Uh oh!

tnull commented Jun 11, 2025

Uh oh!

joostjager commented Jun 11, 2025

Uh oh!

TheBlueMatt commented Jun 11, 2025

Uh oh!

joostjager commented Jun 12, 2025

Uh oh!

joostjager commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joostjager commented Jun 12, 2025

Uh oh!

joostjager commented Jun 12, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joostjager commented May 29, 2025 •

edited

Loading

ldk-reviews-bot commented May 29, 2025 •

edited

Loading

codecov bot commented Jun 2, 2025 •

edited

Loading

joostjager commented Jun 12, 2025 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading

joostjager Jun 12, 2025 •

edited

Loading